Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: OEP-67 automated bundle size checking. #515

Merged
merged 1 commit into from
Dec 12, 2023

Conversation

brian-smith-tcril
Copy link
Contributor

@brian-smith-tcril brian-smith-tcril commented Aug 23, 2023

No description provided.

Copy link
Member

@adamstankiewicz adamstankiewicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As is, these changes LGTM! Left a few non-blocking additional comments/thoughts.


It is important for users of the Open edX platform that we deliver reasonably sized JavaScript bundles. This provides
faster load times to all users, and is vital for users with low bandwidth and/or metered connections. To ensure we
don't unintentionally increase the size of our JavaScript bundles, we should utilize automated bundle size monitoring.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might also be worth a mention that increased observability of bundle size will encourage maintainers to adopt practices such as code/bundle splitting.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


It is important for users of the Open edX platform that we deliver reasonably sized JavaScript bundles. This provides
faster load times to all users, and is vital for users with low bandwidth and/or metered connections. To ensure we
don't unintentionally increase the size of our JavaScript bundles, we should utilize automated bundle size monitoring.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[curious] Is it worth mentioning that we have some support for bundle size analytics today insofar that we do generate a bundle size report via the standard Webpack configurations provided by frontend-build which can be viewed locally? E.g., when you run npm run build, a report.html file is generated in the dist directory that lets developers see bundle sizes of JS assets (compressed vs. uncompressed). However, this report is not very discoverable. Exposing similar data via these CI checks makes bundle size a more prominent concern than with the tools Open edX engineers have to work with today.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it's a good idea to mention it: I myself had totally forgotten about that report!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consequence
***********

`BundleWatch`_ will be adopted by the Open edX community for automatic JavaScript bundle size monitoring.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line feels like it might belong under the "Decision" section?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consequence
***********

`BundleWatch`_ will be adopted by the Open edX community for automatic JavaScript bundle size monitoring.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might also be worth a mention whether it will be up to maintainers / owning squads of repos whether the BundleWatch CI check will be a required status check or not (i.e., informative only versus blocking PRs if exceeding certain threshold). My assumption is that we'd want this CI check to be informative only.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brian-smith-tcril brian-smith-tcril force-pushed the oep11-bundle-size branch 2 times, most recently from 368419e to 6e4ca35 Compare November 20, 2023 17:37
@brian-smith-tcril brian-smith-tcril changed the title docs: OEP-11 automated bundle size checking docs: OEP-67 automated bundle size checking Nov 20, 2023
@feanil feanil changed the title docs: OEP-67 automated bundle size checking docs: OEP-67 automated bundle size checking. Dec 12, 2023
@feanil feanil merged commit a46c4cd into openedx:master Dec 12, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants